-
Notifications
You must be signed in to change notification settings - Fork 830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added executionType parameter to have an ability to run phase jobs SE… #929
Conversation
…QUENTIALLY or PARALLEL
*/ | ||
void phase(String name, String continuationCondition, @DslContext(PhaseContext) Closure phaseClosure) { | ||
PhaseContext phaseContext = new PhaseContext(jobManagement, item, name, continuationCondition) | ||
void phase( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break compatibility as it changes a signature. Leave the signature as it is, just adding a new method to PhaseContext
is sufficient.
PhaseContext(JobManagement jobManagement, Item item, String phaseName, String continuationCondition) { | ||
PhaseContext( | ||
JobManagement jobManagement, Item item, String phaseName, String continuationCondition, | ||
String executionType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add another constructor parameter here. Set the field to the default value.
@@ -36,6 +41,13 @@ class PhaseContext extends AbstractContext { | |||
} | |||
|
|||
/** | |||
* Defines how to run the whole MultiJob phase. | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a @since
tag which points to the next version.
ContextHelper.executeInContext(phaseClosure, phaseContext) | ||
|
||
Preconditions.checkNotNullOrEmpty(phaseContext.phaseName, 'A phase needs a name') | ||
Preconditions.checkArgument( | ||
VALID_CONTINUATION_CONDITIONS.contains(phaseContext.continuationCondition), | ||
"Continuation Condition needs to be one of these values: ${VALID_CONTINUATION_CONDITIONS.join(', ')}" | ||
) | ||
Preconditions.checkArgument( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this check to the executionType
method in PhaseContext
.
@@ -36,6 +41,13 @@ class PhaseContext extends AbstractContext { | |||
} | |||
|
|||
/** | |||
* Defines how to run the whole MultiJob phase. | |||
*/ | |||
void executionType(String executionType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature need version 1.22 of the MultiJob plugin. Add a @RequiresPlugin
annotation.
Suggested fixes has been applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going in the right direction, there are a few things that I forgot to mention last time.
@@ -49,6 +49,7 @@ class MultiJobStepContext extends StepContext { | |||
stepNodes << new NodeBuilder().'com.tikal.jenkins.plugins.multijob.MultiJobBuilder' { | |||
phaseName phaseContext.phaseName | |||
delegate.continuationCondition(phaseContext.continuationCondition) | |||
delegate.executionType(phaseContext.executionType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be wrapped in an if block testing jobManagement.isMinimumPluginVersionInstalled(...)
so that the new element only gets generated if the minimum required plugin version is installed.
thrown(DslScriptException) | ||
|
||
where: | ||
execution << ['FOO'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use a where
section if there is only one option
} | ||
|
||
where: | ||
execution << ['SEQUENTIALLY'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use a where
section if there is only one option, but you could also test both supported options here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good option to modify codenarcTest to catch such conditions
Suggested fixes has been applied |
I've added an ability to switch between execution types in multi jobs step plugin.
This options was missed in job dsl plugin.
Basically it can switch between sequential or parallel execution type of jobs within the phase.